Refactor: Unify DomainAddress and account conversions #1967

lemunozm · 2024-08-14T12:40:04Z

Description

Removed account conversions though Convert traits. Not DomainAddress know about to convert accounts.
Unify DomainAddress to [u8;32]. See this slack thread
- Before this PR you can transform a DomainAddress into [u8;32] in two ways:
  - Using T::DomainAddressIntoAccount::convert() which put 20 bytes from Eth + chain + "EVM" number.
  - Using DomainAddress.address() which put 20 bytes following by 0s.
  This is pretty confusing because is easy to use one or the other in different places, ending with different arrays values. From this PR, the second option is removed. We always add the extra information each time we extract the array from a DomainAddress
  - We need to ensure this is not a problem with out current chain state. From Solidity it seems not because Solidity always reads the first 20 bytes.
Added sugar method to DomainAddress to easy work with them safely
Renamed Centrifuge to Local (or maybe I revert this)
Renamed EVM to Evm
(only in IT). Removed From/Into traits from Keyring to H160 and [u8;20] and use instead an explicit method called in_eth() (open to name proposals). This method represents the account in the Ethereum network. Note that this differs from Keyring::Alice.into()[0..20], which is used in other places where an ethereum account is stored as an AccountId and needs to be recovered. Because this is highly confusing, I've removed the into/from methods to be more explicit about when the first case happens.

lemunozm · 2024-08-14T18:10:01Z

Current implementation is done as:

pub enum DomainAddress {
    Local([u8; 32]),		
    Evm(EVMChainId, [u8; 20]),
}

But I'm thinking of changing this into:

pub enum DomainAddress {
    Centrifuge(AccountId32),		
    Evm(EVMChainId, H160),
}

I do not like that we expose the AccountId32 type (which, theoretically, is set at runtime), but we would gain much more type safety. It's a pain to deal with arrays. Maybe it makes sense to expose an AccountId32 because speaking about a "Centrifuge domain" inherently carries some "domain information"

EDIT: after playing with the second approach, I feel like it's harder to deal with in a lot of scenarios, so I will keep the first model.

codecov · 2024-08-14T20:01:35Z

Codecov Report

Attention: Patch coverage is 60.34483% with 46 lines in your changes missing coverage. Please review.

Project coverage is 47.76%. Comparing base (e8f1efd) to head (764e4b2).

Files	Patch %	Lines
pallets/liquidity-pools/src/hooks.rs	0.00%	10 Missing ⚠️
libs/types/src/domain_address.rs	76.66%	7 Missing ⚠️
node/src/chain_spec.rs	0.00%	6 Missing ⚠️
pallets/liquidity-pools-gateway/src/lib.rs	0.00%	6 Missing ⚠️
pallets/liquidity-pools/src/lib.rs	85.18%	4 Missing ⚠️
runtime/common/src/gateway.rs	0.00%	4 Missing ⚠️
runtime/common/src/routing.rs	0.00%	3 Missing ⚠️
runtime/common/src/account_conversion.rs	0.00%	2 Missing ⚠️
pallets/axelar-router/src/lib.rs	66.66%	1 Missing ⚠️
runtime/altair/src/lib.rs	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1967   +/-   ##
=======================================
  Coverage   47.76%   47.76%           
=======================================
  Files         183      183           
  Lines       12929    12887   -42     
=======================================
- Hits         6176     6156   -20     
+ Misses       6753     6731   -22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

runtime/common/src/gateway.rs

libs/types/src/domain_address.rs

wischli

I wish we did this earlier because this would have saved me quite some time! Overall, looks really good to me. I just have a few nits and couple of questions.

One change request on which I cannot comment: Please revert the submodule change because you are going back one commit there: This PR wants to change 6e8f1a29dff0d7cf5ff74285cfffadae8a8b303f which is origin/main to 4301885b9a3b8ec36f3bda4b789daa5b115c006a

wischli · 2024-08-15T14:29:23Z

libs/types/src/domain_address.rs

 		}
 	}

+	pub fn from_local(address: impl Into<LocalAddress>) -> DomainAddress {


Nit: I am not sure I am fully happy with the naming here because the returned domain address implicitly local even though it could also be EVM from a 32 byte array input.

If the user uses a 32-byte array (that represents an eth address expanded to 32 bytes) and sets this as local, then they are using the method wrongly. Or if not wrongly, they are creating an account from an array with X data, where X is a eth address in the array.

Not sure if I got what you want to say.

wischli · 2024-08-15T14:30:18Z

libs/types/src/domain_address.rs

+pub type LocalAddress = [u8; 32];
+pub type EthAddress = [u8; 20];
+
+pub fn local_to_eth_address(address: LocalAddress) -> EthAddress {


Nit: Let's stick to one namespace for Eth, Evm, Solidity

Suggested change

pub fn local_to_eth_address(address: LocalAddress) -> EthAddress {

pub fn local_to_evm_address(address: LocalAddress) -> EthAddress {

I wanted to distinguish for the name:

eth: just the address

evm: and ethereum address + chain information

I say this because not sure for example if [u8; 20] is an evm address, I think not because it does not have chain information 🤔

But not sure if this association makes sense TBH. It seems not 😅

I definitely get what you are saying but to me, having two namespaces just causes confusion.

However, since he address format is referred to as the Ethereum address format (as least from Metamask), it appears to me your distinction is correct. So fine with keeping as is.

Will add a comment regarding this then to explain the why. Thanks for the ref.

I think the key sentence is: in a EVM there are ETH address, so evm makes sense for domains, and eth for addresses

Frontier's approach is pretty straightforward when it comes to eth/evm addresses, they name these AccountId20 - here.

As someone who didn't have to deal much with Domain/DomainAddress, I find that local/eth/evm are confusing as well tbh.

Why not go for something simpler like:

CentrifugeAccount - AccountId32.

EvmAccount - AccountId20/[u8;20].

CentrifugeEvmAccount - AccountId32 that's basically EvmAccount + chain_id + tag.

Then we could have implementations for:

CentrifugeAccount -> EvmAccount - truncate;

EvmAccount -> CentrifugeAccount - pad with 0s;

CentrifugeAccount <-> CentrifugeEvmAccount - no need;

EvmAccount -> CentrifugeEvmAccount - append chain_id + tag;

CentrifugeEvmAccount -> EvmAccount - truncate;

Thanks for sharing this!

And these CentrifugeAccount, etc, should be types on their own, right? So you would directly remove the DomainAddress type? Which type will you use to track all of them generically?

No, I would add them to the DomainAddress:

pub enum DomainAddress { /// A standard Centrifuge account. Centrifuge(CentrifugeAccount), /// A Centrifuge account derived from an `EvmAccount`. CentrifugeEvm(CentrifugeEvmAccount), /// An EVM-compatible account. Evm(EvmAccount), }

This way we can keep the above mentioned conversions, and just have From implementations for DomainAddress, and for each of these account types.

Ok, I like the way you're taking it.

Just a problem with this. The variant Evm(EvmAccount) will not have a Domain information associated with it. A DomainAddress requires it to later extract this.

For sure, I would not have time to do such a refactor for Monday 😅

We could add a struct for the evm account that has the chain and the actual account, or keep it as it is right now? I dunno, just thinking out loud at this point ^^

But that would be what we have right now, right? 🤔

I'm going to sketch a new draft with some ideas from this, but reusing the current DomainAddress, let's see. Thanks for your thoughts!

wischli · 2024-08-15T14:30:42Z

libs/types/src/domain_address.rs

-		let decoded = Domain::decode(&mut encoded.as_slice()).unwrap();
-
-		assert_eq!(domain, decoded);
+	pub fn as_eth<Address: From<EthAddress>>(&self) -> Address {


Nit: Namespace

Suggested change

pub fn as_eth<Address: From<EthAddress>>(&self) -> Address {

pub fn as_evm<Address: From<EthAddress>>(&self) -> Address {

Thinking now about just eth/evm, WDYT?

pallets/liquidity-pools/src/hooks.rs

wischli · 2024-08-15T14:38:07Z

pallets/liquidity-pools/src/message.rs


 		test_encode_decode_identity(
 			Message::TransferTrancheTokens {
 				pool_id: 1,
 				tranche_id: default_tranche_id(),
 				domain: domain_address.domain().into(),
-				receiver: domain_address.address(),
+				receiver: domain_address.as_local(),


I am skeptical about the changed serialization here. Probably should be

Suggested change

receiver: domain_address.as_local(),

receiver: domain_address.as_eth(),

address() returned a [u8; 32], the receiver has type [u8; 32] but as_eth() returns an [u8;20].

Basically we use address as 32-bytes in the message for compatibility reasons, but later the 12 remaining bytes are discarded by solidity, see this thread: https://kflabs.slack.com/archives/C06FRHCRQL9/p1723627567780339

Thanks for breaking this down. Seems like the message serialization is now correct and was poorly mocked beforehand, i.e. by using the 20 byte account address which in reality, should never occur.

Yeah, previously it was a chaos of different versions of [u8; 20] -> [u8; 32] conversions

pallets/liquidity-pools/src/mock.rs

wischli · 2024-08-15T14:39:40Z

pallets/liquidity-pools/src/mock.rs

-	AccountId::new(arr)
-};
-pub const CENTRIFUGE_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Centrifuge(ALICE_32);
+pub const ALICE_EVM_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Evm(42, ALICE_ETH);


You re-inserted the incorrect chain ID which was fixed in #1964

Suggested change

pub const ALICE_EVM_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Evm(42, ALICE_ETH);

pub const ALICE_EVM_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Evm(CHAIN_ID, ALICE_ETH);

I should do something wrong in the rebase, although I do not know how it passes then, I will inspect

Ok, this has changed because ALICE_EVM_LOCAL_ACCOUNT no longer exists, and now we're using ALICE_EVM_DOMAIN_ADDRESS, which has chain 42, not sure why TBH...

Ah ok, never mind... fixing it

Thanks for being aware of this!

pallets/liquidity-pools/src/tests.rs

runtime/common/src/gateway.rs

wischli · 2024-08-15T14:48:21Z

runtime/integration-tests/src/utils/accounts.rs

+	/// Returns the Ethereum address.
+	/// The H160 retrived is NOT the local representation of that account in our
+	/// chain, it's the real address in ethereum.
+	pub fn in_eth(self) -> H160 {


Nit: Namespace

Suggested change

pub fn in_eth(self) -> H160 {

pub fn in_evm(self) -> H160 {

Will change if you confirm the above makes no sense, that probably not 😆

lemunozm · 2024-08-15T15:18:03Z

This PR wants to change 6e8f1a29dff0d7cf5ff74285cfffadae8a8b303f which is origin/main to 4301885b9a3b8ec36f3bda4b789daa5b115c006a

Good point! It was during the rebase...

lemunozm · 2024-08-15T15:41:09Z

Given the change from Local to Centrifuge, I should also change the as_local() and from_local(), I think both could be: as_account() (or just account()) and from_account().

Do you agree or do you have some preference? @wischli @cdamian

wischli · 2024-08-15T15:55:22Z

Given the change from Local to Centrifuge, I should also change the as_local() and from_local(), I think both could be: as_account() (or just account()) and from_account().

Do you agree or do you have some preference? @wischli @cdamian

Sounds good to me! {as, from}_centrifuge certainly sounds wrong. If we want to be more explicit, we could also use ..account32.. or ..account_id...

lemunozm · 2024-08-15T16:36:55Z

Well, I need to think a bit more because what's returned is the 32-bit array, so the account name may not make sense in some places where the array is expected (as when we construct a message with an eth address expanded to 32 bytes). Such a dilemma!

lemunozm · 2024-08-15T23:39:28Z

Closed in favor of: #1969

lemunozm self-assigned this Aug 14, 2024

lemunozm force-pushed the ref/domain-address branch from af9bc6c to f2f9db5 Compare August 14, 2024 15:30

lemunozm marked this pull request as ready for review August 14, 2024 18:10

lemunozm requested review from wischli and cdamian and removed request for wischli August 14, 2024 22:33

lemunozm changed the title ~~Refactor: Unify DomainAddress and account conversions (WIP)~~ Refactor: Unify DomainAddress and account conversions Aug 15, 2024

lemunozm commented Aug 15, 2024

View reviewed changes

runtime/common/src/gateway.rs Show resolved Hide resolved

lemunozm added 10 commits August 15, 2024 12:51

runtime-common compiling

9cac0c1

runtimes compiling

a76eba5

add IT required changes

634d2ee

add id_eth

6db6ca9

rename id_eth to in_eth

ca9efa0

fix lp UTs, still pending the inbounds

95d9148

finish lp UTs

81ee4a5

fix IT last tests

6bbf27d

fix clippy and benchmarking

5825728

fix issues after rebasing

84c03a8

lemunozm force-pushed the ref/domain-address branch from 279d48e to 84c03a8 Compare August 15, 2024 11:02

fix IT

764e4b2

cdamian reviewed Aug 15, 2024

View reviewed changes

libs/types/src/domain_address.rs Show resolved Hide resolved

wischli reviewed Aug 15, 2024

View reviewed changes

lemunozm mentioned this pull request Aug 15, 2024

Refactor: Unify DomainAddress and account conversions (r2) #1969

Merged

lemunozm closed this Aug 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Unify DomainAddress and account conversions #1967

Refactor: Unify DomainAddress and account conversions #1967

lemunozm commented Aug 14, 2024 •

edited

Loading

lemunozm commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

wischli left a comment

wischli Aug 15, 2024

lemunozm Aug 15, 2024

wischli Aug 15, 2024

lemunozm Aug 15, 2024 •

edited

Loading

wischli Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading

cdamian Aug 15, 2024

lemunozm Aug 15, 2024

cdamian Aug 15, 2024

lemunozm Aug 15, 2024

cdamian Aug 15, 2024

lemunozm Aug 15, 2024 •

edited

Loading

wischli Aug 15, 2024

lemunozm Aug 15, 2024

wischli Aug 15, 2024

lemunozm Aug 15, 2024 •

edited

Loading

wischli Aug 15, 2024

lemunozm Aug 15, 2024

wischli Aug 15, 2024

lemunozm Aug 15, 2024

lemunozm Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024

lemunozm Aug 15, 2024

wischli Aug 15, 2024

lemunozm Aug 15, 2024

lemunozm commented Aug 15, 2024

lemunozm commented Aug 15, 2024

wischli commented Aug 15, 2024

lemunozm commented Aug 15, 2024

lemunozm commented Aug 15, 2024

	pub fn local_to_eth_address(address: LocalAddress) -> EthAddress {
	pub fn local_to_evm_address(address: LocalAddress) -> EthAddress {

	pub fn as_eth<Address: From<EthAddress>>(&self) -> Address {
	pub fn as_evm<Address: From<EthAddress>>(&self) -> Address {

	receiver: domain_address.as_local(),
	receiver: domain_address.as_eth(),

	pub const ALICE_EVM_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Evm(42, ALICE_ETH);
	pub const ALICE_EVM_DOMAIN_ADDRESS: DomainAddress = DomainAddress::Evm(CHAIN_ID, ALICE_ETH);

Refactor: Unify DomainAddress and account conversions #1967

Refactor: Unify DomainAddress and account conversions #1967

Conversation

lemunozm commented Aug 14, 2024 • edited Loading

Description

lemunozm commented Aug 14, 2024 • edited Loading

codecov bot commented Aug 14, 2024 • edited Loading

Codecov Report

wischli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

wischli Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

lemunozm Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemunozm commented Aug 15, 2024

lemunozm commented Aug 15, 2024

wischli commented Aug 15, 2024

lemunozm commented Aug 15, 2024

lemunozm commented Aug 15, 2024

lemunozm commented Aug 14, 2024 •

edited

Loading

lemunozm commented Aug 14, 2024 •

edited

Loading

codecov bot commented Aug 14, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading

wischli Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading

lemunozm Aug 15, 2024 •

edited

Loading